Fix in-process callback reclamation after connection close - #2610

Cross-SDK Consistency Review — PR #2610

I compared the FFI runtime host lifecycle changes across all six SDKs (dotnet/src/FfiRuntimeHost.cs, go/internal/ffihost/ffihost.go, java/sdk/.../FfiRuntimeHost.java + JnaNativeBinding.java, nodejs/src/ffiRuntimeHost.ts, python/copilot/_ffi_runtime_host.py, rust/src/ffi.rs).

Verdict: consistent. This PR replaces the old "count active callbacks and spin-wait" drain strategy with a unified pattern in all six languages:

• connection_close is now treated as the authoritative callback-quiescence signal. If it returns "not yet quiescent," cleanup (releasing the callback/host shutdown) is deferred and retried on a 100ms interval — the same interval constant is used everywhere (CleanupRetryDelayMilliseconds/CLEANUP_RETRY_INTERVAL_MS/_CLEANUP_RETRY_INTERVAL_SECONDS = 0.1/Go & Rust 100 * time.Millisecond / Duration::from_millis(100)).
• host_shutdown returning false is now logged (debug/fine level) but treated as terminal — no retry — consistently in .NET, Go, Java, Node, and Python.
• Startup now re-checks the disposed flag after the blocking native host_start/connection_open calls to correctly finalize cleanup if dispose()/Dispose()/close() raced with startup (.NET, Node, Python, Java all do this explicitly; Go achieves the same effect structurally since Start() and Dispose() share lifecycleMu and thus can't run concurrently; Rust's ownership model prevents the race by construction since no Arc<FfiShared> handle exists until after native setup completes).
• A best-effort "quarantine" set is added in .NET, Java, Node, and Python to retain hosts whose native connection_close call itself threw/raised, since those runtimes can catch exceptions crossing the FFI boundary. Go and Rust don't add an equivalent, but that's consistent with their FFI conventions — both native bindings communicate exclusively via bool return values with no thrown-exception path, so there's nothing analogous to catch. This is a case of a legitimate language-specific difference (per the review guidelines' "language-specific optimization" carve-out), not a missing feature.
• Each language ships new/updated unit tests exercising the "retains state until connection_close succeeds" and "does not retry a terminal host_shutdown failure" scenarios (FfiRuntimeHostLifetimeTests.cs, ffihost_test.go, FfiRuntimeHostTest.java, ffiRuntimeHost.test.ts, test_ffi_runtime_host.py, and the new #[test] callback_state_is_retained_until_connection_close_succeeds in rust/src/ffi.rs).

Naming follows each language's idiom (TryFinalizeNativeCleanup/tryFinalizeCleanupLocked/tryFinalizeCleanup/_try_finalize_cleanup/try_finalize_cleanup), and public API surface (start/dispose/close) is unchanged in every SDK.

No action items — nice work keeping this in lockstep across all six implementations.

Generated by SDK Consistency Review Agent for #2610 · copilot · sonnet50 · 93 AIC · ⌖ 12.3 AIC · ⊞ 8.3K · ◷

@SteveSandersonMS Thanks for digging into this. The key distinction is queued replies vs. a C callback that is already executing.

copilot_runtime_connection_close is intentionally an untracked/discarding close. It latches the outbound gate and discards frames that have not entered the callback yet, so 2,000 outstanding requests do not by themselves make it return false. A first-call true in that scenario is expected. Likewise, higher-level replies can settle after close if their bytes had already crossed the callback boundary and were buffered in JS before close; the guarantee is about the C callback and its user_data, not completion of downstream stream/RPC processing.

The receiveStream.on("data") test is closer to the interesting case, but its assertion cannot distinguish the old and new SDK behavior. In Koffi 3.2.1, a callback invoked from a foreign thread calls napi_call_threadsafe_function(..., napi_tsfn_blocking) and then waits on a condition variable until the JS callback finishes (CallData::RelayAsync). The runtime's cabi-outbound thread therefore remains inside OutboundGate::dispatch while the JS data listener calls dispose(). Since close runs on the JS thread rather than the native dispatcher thread, the runtime waits up to its 5-second quiescence budget, returns false, and the callback finishes only after dispose() returns. On old SDK code, that false was ignored and the Koffi registration was unregistered anyway; that is a contract violation, but unregistering the callback that is already executing does not deterministically crash because Koffi has already loaded the JS function reference for that invocation. Timing the dispose() call should show the roughly 5-second timeout, but merely surviving does not prove reclamation was safe.

The deterministic bad window is narrower: native has entered/dispatched the callback, but the host has not yet safely acquired its callback state. A public SDK E2E cannot reliably pause at that instruction boundary without an instrumentation hook in the native callback. That is why the runtime change added event-driven gate tests and why these SDK tests inject false then true: they verify the host side of the now-explicit ABI contract rather than trying to turn a use-after-free race into a reliable black-box crash. The intermittent .NET CI failure (all 232 tests passed, then the testhost crashed during teardown) is consistent evidence, but I agree it is not a deterministic repro by itself.

On Rust specifically, removing active_callbacks strengthens the guarantee. The old counter increments inside on_outbound, after native has already dispatched the callback and passed the raw pointer. A close can observe zero and free the box while that callback is paused before its first increment/dereference; the callback then resumes into freed memory. Waiting for the counter only covers callbacks that already completed that unsafe dereference. The runtime gate increments in_flight before invoking the host callback and returns true only after it reaches zero, closing exactly that gap. Keeping the host counter would be redundant after a successful close and still would not repair the pre-increment race.

The authoritative contract and implementation are in github/copilot-agent-runtime@8228fb4bbc8: OutboundGate::dispatch records in_flight before invoking the host, OutboundGate::close returns Reentrant/TimedOut without authorizing reclamation, and copilot_runtime_connection_close returns true only for a known, quiescent gate. The commit message also documents the original Rust SDK teardown corruption and the runtime regressions that fail against the pre-fix implementation.

So I don't think we should add a slow, timing-based E2E that passes on both implementations. The focused adapter tests are intentionally contract tests for the only observable branch each SDK must handle: retain ownership on false, reclaim exactly once after a later true.

Generated by Copilot